Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support pose,object constraints #63

Merged
merged 2 commits into from
Aug 3, 2018
Merged

Support pose,object constraints #63

merged 2 commits into from
Aug 3, 2018

Conversation

Jason0214
Copy link
Collaborator

@Jason0214 Jason0214 commented Jul 30, 2018 •
edited
Loading

intent to solve #62 #38

it already worked on simple test case, though still several limitations

  • the python API I used to bake action is active in development, what I used is compatible with blender 2.79 last year, however, it seems being changed in the latest blender 2.79
  • currently if the object has no animation data, it won't go into the baking function, it needs to be solved
  • baking action needs explicitly passing the frame_begin, frame_end to do baking, however, it is hard to decide
  • need test on action in nla_stack

@Jason0214 Jason0214 mentioned this pull request Jul 31, 2018
Closed
@Jason0214 Jason0214 force-pushed the support_constraint_action branch 3 times, most recently from fa6da45 to 31f9e46 Compare July 31, 2018 21:47
@Jason0214 Jason0214 changed the title [WIP ]support pose,object constraints support pose,object constraints Jul 31, 2018
@Jason0214
Copy link
Collaborator Author

Jason0214 commented Jul 31, 2018 •
edited
Loading

have a review @sdfgeoff 🎉

there are three commits, so I just make a brief explanation:

  1. bake the constraint of object to action, so we can have support for IK
  2. previously I placed AnimationPlayer as sibling of animated object, I found it may cause the whole scene has only one AnimationPlayer, which is not good. So I placed it to child of animated object
  3. remove some redundant check for existence of animation_data

as you can see, there is one minor problem I still not solved,
in blender 2.79.0 bake_action
in blender 2.79.1 bake_action
the difference is the old one grab object from scene.context, the new one explicitly pass the object to function. Do you have any idea how we can support both?

sdfgeoff
sdfgeoff reviewed Aug 2, 2018
View reviewed changes
io_scene_godot/converters/animation.py
@@ -141,15 +142,16 @@ class AnimationPlayer(NodeTemplate):
def __init__(self, name, parent):
super().__init__(name, "AnimationPlayer", parent)
# use parent node as the animation root node
self['root_node'] = NodePath(self.get_path(), self.parent.get_path())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious as to why parent and self.parent are different. From what I can see of NodeTemplate, they should be the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it should be the same. While I develop on this PR, I removed it first and then add it back, so caused some inconsistence, I will fix it

sdfgeoff
sdfgeoff reviewed Aug 2, 2018
View reviewed changes
io_scene_godot/converters/animation.py Outdated
need_bake = action_type == 'transform' and (has_obj_cst or has_pose_cst)

def action_baker(action_to_bake):
"""baker for transform action, note it is a closure"""
Copy link
Collaborator

@sdfgeoff sdfgeoff Aug 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite a closure. Sure, it's using the environment from the parent function but it isn't ever used outside it's parent function. If you were (for whatever reason) returning the action_baker function from the end of export_animation_data, then it would be a closure.

I would classify this just as a "nested function"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, I just want to keep a note that this function use variable outside

@sdfgeoff
Copy link
Collaborator

sdfgeoff commented Aug 2, 2018

To support both you could run a check against bpy.app.version

@Jason0214
Copy link
Collaborator Author

After I check the source code of action_bake https://developer.blender.org/diffusion/B/browse/master/release/scripts/modules/bpy_extras/anim_utils.py, I surprisingly find the action_bake call frame_set() to bake, so if we doing baking for every object, it is a waste of performance. Instead, we should gathering all the objects need action baking and then call action baking for once. But I'd rather want this PR merged first (as baking action is eagerly wanted) and then open a issue to solve the performance problem

@Jason0214 Jason0214 changed the title support pose,object constraints Support pose,object constraints Aug 2, 2018
sdfgeoff
sdfgeoff reviewed Aug 3, 2018
View reviewed changes
io_scene_godot/converters/animation.py
do_visual_keying=True,
)

bpy.context.scene.objects.active = active_obj_backup
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably not be only for blender 2.79!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, it should be place outside, I am so careless..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after think about a bit, I moved the active_obj_backup into the if block instead of move this line outside

@sdfgeoff
Copy link
Collaborator

sdfgeoff commented Aug 3, 2018

Looking pretty good now. I think you can squash (after fixing the indentation of restoring the active object).

@Jason0214 Jason0214 force-pushed the support_constraint_action branch from edc7b85 to f3df39d Compare August 3, 2018 07:41
@Jason0214
Copy link
Collaborator Author

I prefer to keep it as two commits (because they are separate two). then if something wrong happen, it is easy to drop one

@sdfgeoff
Copy link
Collaborator

sdfgeoff commented Aug 3, 2018

Yup, makes sense.

@sdfgeoff sdfgeoff merged commit 5b6767b into godotengine:master Aug 3, 2018
@guilhermefelipecgs guilhermefelipecgs mentioned this pull request Aug 5, 2018
Closed
This was referenced Aug 6, 2018
Merged
Open
@Jason0214 Jason0214 deleted the support_constraint_action branch August 10, 2018 20:32
Ben1138 pushed a commit to Ben1138/godot-blender-exporter that referenced this pull request Oct 2, 2024
@sdfgeoff
Merge pull request godotengine#63 from Jason0214/support_constraint_a…
d20d78b 
…ction

Support pose,object constraints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdfgeoff sdfgeoff sdfgeoff left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jason0214 @sdfgeoff